Onboarding Project: Please Review #29
Conversation
…nor aesthetic modifications.
JacoBriers
left a comment
JacoBriers
left a comment
There was a problem hiding this comment.
Some general comments:
- I like the look and feel, nice and clean
- On the navigation: splitting the search and navigate-to-page functionality that you provide with the "Enter ID" input into two inputs, may make it a bit clearer to the user what they are for.
- Code neatness needs a little work: consistent use of tabs and spaces, use of white-space between variables and operators, etc. There are plugins for IDEs that will help you out here.
Well done!
app.ts
Outdated
| function buildTable(columns, records) { | ||
|
|
||
| // create table | ||
| let table = document.createElement("table"); |
There was a problem hiding this comment.
Consistently use tabs/spaces for formatting. We set up VSCode for our projects to auto-format our code, so this isn't usually a problem, but if you don't have help from the IDE you have to do it manually. Formatting-war is a real thing and we do all we can to prevent it :)
app.ts
Outdated
| table.appendChild(thead); | ||
|
|
||
| // create rows | ||
| records.forEach(function (el) { |
There was a problem hiding this comment.
Rather use for of loop or old fashioned for loop
app.ts
Outdated
| // debounce function to reduce frequency of queries made | ||
| window.onresize = () => { | ||
| clearTimeout(resizeTimer); | ||
| resizeTimer = setTimeout(function(){ |
app.ts
Outdated
| record.style.display="block"; | ||
| if(td){ | ||
| let val = td.innerText || td.textContent; | ||
| if(val.indexOf(input) > -1){ |
There was a problem hiding this comment.
Don't overuse ternary statements but here you could:
record.style.display = val.indexOf(input) === -1 ? "none" : "";
app.ts
Outdated
| loader.innerHTML=`<div class="loader" id="loader"></div>`; | ||
|
|
||
| // outter function to fetch column headers from server | ||
| $.get("http://localhost:2050/columns", function(columns){ |
There was a problem hiding this comment.
This call will always return the same values, so you can call it once and cache the results, like you did with record count.
app.css
Outdated
| border-top: 16px solid #3498db; | ||
| width: 120px; | ||
| height: 120px; | ||
| -webkit-animation: spin 2s linear infinite; /* Safari */ |
There was a problem hiding this comment.
Did you test this on Safari?
There was a problem hiding this comment.
No I didn't - will remove it for now since it's not necessary to include.
app.ts
Outdated
| if(e.which == 13) | ||
| { | ||
| let end = parseInt(input) + calculateRows(); | ||
| clearTable(); |
There was a problem hiding this comment.
Since you always clearTable before you loadPage you can move clearPage inside loadPage
app.ts
Outdated
| } | ||
|
|
||
| // loads the columns and rows to be displayed based on start and end row indices | ||
| function loadPage(start:number, end:number){ |
There was a problem hiding this comment.
We could make this take only the start index and then you add caclulateRows() inside this method to calc the end index.
|
|
||
| // filters rows by user input ID | ||
| function search(e){ | ||
| let input = $("#search").val().toString(); |
There was a problem hiding this comment.
It isn't a problem here, but when working in the codebase for our other projects, you will need to use more specific div IDs since they are in a "global namespace" of sorts and could conflict with the naming of existing divs.
|
Thanks for the feedback, Jaco - will submit another pull request in due course once every recommendation has been accounted for. |
…on with Safari removed.
2 participants
No description provided.